refactor(export): drop send_rgba and the state that existed only for it (Linux) - #580
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthrough
ChangesYUV420P encoder input
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Linux export now accepts GPU-produced YUV420P frames directly and removes the unused RGBA conversion path. The updated test exercises the production-style frame copy path, with no current merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description clearly explains the refactor, test migration, content-pattern change, cleanup, and verification results. However, it omits the required template sections for Related issue, Type of change, Release impact, Desktop impact, Screenshots / video, and Testing headings or checkboxes. Resolution Reformat the description with all required template sections. Identify the related issue references, select Refactor / maintenance and the applicable release and Linux impact options, state whether screenshots or video are not applicable, and place the test commands and end-to-end verification under Testing. ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…it (Linux) Net −61 lines. `send_rgba` had no production caller once the YUV conversion moved to the GPU: the export feeds the encoder through `copy_into` from a GPU-strided pool frame, and nothing else called it. It was not quite dead, which is why this is a refactor rather than a deletion. The tests added with the keyframe fix used it to push frames in — so they were verifying the encoder's configuration through a path production no longer takes. They now feed through `copy_into` and a `YuvLayout` buffer, which is what the export actually does, so they exercise the real thing rather than a parallel one. That migration needed a content change, and the reason is worth recording: the first pattern I used gave keyframes at 0/17/34 instead of 0/20/40 — a contrasty motif wakes openh264's scene-change detection, and the test would then have been measuring that rather than `gop_size`. It now writes a coarse, low-amplitude gradient sliding one step per frame: enough residual to make the encoder work, not enough discontinuity to trigger a scene cut. With `send_rgba` gone, the compiler found the rest: `VideoEncoder::sw` (a 3.1 MB AVFrame allocated per encoder and never written since the pool took over), the `sws: *mut SwsContext` beside it, `SWS_POINT`, and `alloc_sw_frame`. All removed, and `VideoEncoder::open` no longer has a fallible step after `avcodec_open2`, so its error branch went with them. 215 tests pass; both export paths verified end to end, 3600 frames each.
530338b to
427ed4f
Compare
1 participant
Net −61 lines.
send_rgbalost its production caller when the YUV conversion moved to the GPU (#555): the export feeds the encoder throughcopy_intofrom a GPU-strided pool frame.It was not quite dead, though, which is why this is a refactor and not a deletion. The tests added with #575 used it to push frames in — so they were verifying the encoder configuration through a path production no longer takes. They now feed through
copy_intoand aYuvLayoutbuffer, i.e. exactly what the export does.That migration needed a content change, and it is the interesting part. My first replacement pattern produced keyframes at 0/17/34 instead of 0/20/40: a contrasty motif wakes openh264 scene-change detection, and the test would then have been measuring that rather than
gop_size. It now writes a coarse, low-amplitude gradient sliding one step per frame — enough residual for the encoder to work on, not enough discontinuity to trigger a cut.With
send_rgbagone the compiler found the rest:VideoEncoder::sw— a 3.1 MB AVFrame allocated per encoder, unwritten since the pool took oversws: *mut SwsContextbeside itSWS_POINTalloc_sw_frameVideoEncoder::openno longer has a fallible step afteravcodec_open2, so its error branch went too.Verified: 215 tests pass, and both export paths run end to end — hardware and software, 3600 frames each.
Summary by CodeRabbit
Performance
Compatibility